Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MenuItem][Joy] Add internal id to DOM #37361

Closed
wants to merge 4 commits into from
Closed

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented May 22, 2023

Inspect first option of both sandboxes and you can see id is added to dom only in after sandbox

before: https://codesandbox.io/s/quirky-chaplygin-5skfj3?file=/demo.tsx
after: https://codesandbox.io/s/nervous-pond-l5cfzp?file=/demo.tsx

This PR fixes

@sai6855 sai6855 requested a review from siriwatknp May 22, 2023 07:53
@sai6855 sai6855 added bug 🐛 Something doesn't work component: menu This is the name of the generic UI component, not the React module! design: joy This is about Joy Design, please involve a visual or UX designer in the process labels May 22, 2023
@sai6855 sai6855 marked this pull request as draft May 22, 2023 07:55
@mui-bot
Copy link

mui-bot commented May 22, 2023

Netlify deploy preview

https://deploy-preview-37361--material-ui.netlify.app/

Bundle size report

Details of bundle changes

Generated by 🚫 dangerJS against 6dcd770

@sai6855 sai6855 added package: joy-ui Specific to @mui/joy and removed design: joy This is about Joy Design, please involve a visual or UX designer in the process labels May 22, 2023
@sai6855 sai6855 marked this pull request as ready for review May 22, 2023 08:28
@sai6855 sai6855 marked this pull request as draft May 22, 2023 08:44
@@ -184,6 +184,20 @@ describe('Joy <MenuItem />', () => {
expect(menuitem).to.have.attribute('aria-disabled', 'true');
});

it('should add external id to DOM and override internal id', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is passing in master too but i just wanted to add a test which tests adding extrenal id to DOM when available

@sai6855 sai6855 marked this pull request as ready for review May 22, 2023 09:28
@sai6855 sai6855 changed the title [MenuItem][Joy] Accept external id [MenuItem][Joy] Accept internal id to DOM May 22, 2023
@sai6855 sai6855 changed the title [MenuItem][Joy] Accept internal id to DOM [MenuItem][Joy] Add internal id to DOM May 22, 2023
@sai6855
Copy link
Contributor Author

sai6855 commented May 22, 2023

closing in favour of #37364

@sai6855 sai6855 closed this May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: menu This is the name of the generic UI component, not the React module! package: joy-ui Specific to @mui/joy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants